GH-48728: [C++] Cache compiled regex matchers in string kernels #48729
Conversation
|
|
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for this. This ties the cached object's lifetime to the lifetime of the KernelState, right?
The problem is that a new KernelState is created each time GetFunctionExecutor is called (through KernelState::Init). So in most cases the caching would not be very effective. I suppose the caching may work for chunked array inputs, though?
| // Similar to OptionsWrapper, but caches a compiled object to avoid recompiling on each | ||
| // invocation (e.g., regex matchers). Follows the same pattern as OptionsWrapper. | ||
| template <typename OptionsType> | ||
| struct CachedOptionsWrapper : public KernelState { |
There was a problem hiding this comment.
Can this perhaps inherit from OptionsWrapper if it can reduce the amount of additional code?
| // Get or create cached object of a specific type | ||
| template <typename ObjectType, typename... Args> | ||
| Result<const ObjectType*> GetOrCreate(Args&&... args) { |
There was a problem hiding this comment.
Since this is meant to be independent of ObjectType, do you think it's worth to make it take a callable to avoid relying on the existence of ObjectType::Make?
Something like this perhaps:
| // Get or create cached object of a specific type | |
| template <typename ObjectType, typename... Args> | |
| Result<const ObjectType*> GetOrCreate(Args&&... args) { | |
| // Get or create cached object of a specific type | |
| template <typename ObjectType, typename... Args> | |
| auto GetOrCreate(Factory&& factory, Args&&... args) | |
| -> Result<const std::decay_t<decltype(factory(args...))>*> { |
| // Type-erased cache for compiled objects (can store any object type) | ||
| std::shared_ptr<void> cached_object; |
github-actions
bot
added
awaiting committer review
2 participants
Rationale for this change
String operations with regex patterns (match, replace, extract) were recompiling regex patterns on every invocation. This PR implements caching to compile once and reuse.
Benchmark shows roughly 36% performance improvement (2.52s -> 1.61s for 200 operations).
arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc
Line 1371 in 727106f
arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc
Line 1381 in 727106f
arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc
Line 1965 in 727106f
arrow/cpp/src/arrow/compute/kernels/scalar_string_ascii.cc
Line 2218 in 727106f
What changes are included in this PR?
CachedOptionsWrapper<T>template for kernel state with caching supportMatchSubstringState,ReplaceState, andExtractRegexStateto use cachingExec()methods to callGetOrCreate<Matcher>()instead of directMatcher::Make()Are these changes tested?
Yes. All existing tests pass. Benchmark demonstrates measurable performance improvement when same pattern is used across multiple operations.
(Generated by ChatGPT)
Benchmark:
Results:
Are there any user-facing changes?
No, this is an optiomization.